Skip to content

Comments

Support DateTime instances#231

Closed
Tigrov wants to merge 1 commit intoyiisoft:masterfrom
Tigrov:support_datetime_instances
Closed

Support DateTime instances#231
Tigrov wants to merge 1 commit intoyiisoft:masterfrom
Tigrov:support_datetime_instances

Conversation

@Tigrov
Copy link
Member

@Tigrov Tigrov commented Aug 5, 2023 •
edited
Loading

Support for the time type with test coverage will be added after review of #230

Q A
Is bugfix? ❌
New feature? ✔️
Breaks BC? ✔️
Fixed issues #725

@what-the-diff
Copy link

what-the-diff bot commented Aug 5, 2023

PR Summary

  • Enhanced ColumnSchema class with a new method
    This change introduces a phpTypecast method to the ColumnSchema class, paving the way for a more versatile use of the class in different scenarios.

  • Enhanced Connection class
    We've added a new method initConnection to the Connection class. This would help in setting up the initial configurations for the database connection with ease.

  • Advanced datetime handling
    Through the addition of getDateTimeFormat in the Schema class and modifying the createColumnSchema method, we're now more adept in handling time and date data types. The normalizeDefaultValue modification further optimizes how we handle default values for timestamps.

  • Improved testing
    We're enforcing code quality and reliability by introducing new tests to ColumnSchemaTest and SchemaProvider. These additions ensure our methods for handling different column types and their attributes are thoroughly checked.

  • Updated SQL fixture
    The oci.sql fixture file has seen changes, too, to support columns with different default values for timestamps and dates. This change makes our test data align better with the real-world scenarios.

@codecov
Copy link

codecov bot commented Aug 5, 2023 •
edited
Loading

Codecov Report

Attention: Patch coverage is 97.05882% with 1 line in your changes missing coverage. Please review.

Project coverage is 98.21%. Comparing base (744eb43) to head (1269d82).
Report is 93 commits behind head on master.

Files with missing lines Patch % Lines
src/ColumnSchema.php 83.33% 1 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##             master     #231      +/-   ##
============================================
- Coverage     98.29%   98.21%   -0.08%     
- Complexity      186      194       +8     
============================================
  Files            16       16              
  Lines           585      617      +32     
============================================
+ Hits            575      606      +31     
- Misses           10       11       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Tigrov Tigrov marked this pull request as ready for review August 5, 2023 16:19
darkdef
darkdef reviewed Aug 6, 2023
View reviewed changes
Copy link
Contributor

@darkdef darkdef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need restore code coverage

src/ColumnSchema.php
return parent::phpTypecast($value);
}

public function hasTimezone(): bool
Copy link
Contributor

@darkdef darkdef Aug 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need have a tests for public functions

Copy link
Member Author

@Tigrov Tigrov Aug 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ColumnSchema::hasTimezone() can be replaced with two new abstract types SchemaInterface::TYPE_TIMESTAMPTZ and SchemaInterface::TYPE_TIMETZ

Tests will be added after review this option in yiisoft/db #736

src/Connection.php
parent::initConnection();

$this->pdo->exec(
<<<SQL
Copy link
Contributor

@darkdef darkdef Aug 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What benefits in changes settings of db? What about cases with default settings?
This changes can be in documentation only. It's change user experience.

Copy link
Member Author

@Tigrov Tigrov Aug 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When getting datetime values from Oracle they are returned in a specific format that is not suitable for proper parsing by DateTime.
Yes, sure it will need to be added to the documentation

@Tigrov
Copy link
Member Author

Tigrov commented Aug 6, 2023

Need restore code coverage

Yes, mentioned about this in the description:

Support for the time type with test coverage will be added after review of #230

@samdark samdark requested a review from terabytesoftw August 11, 2023 14:05
@Tigrov Tigrov marked this pull request as draft August 17, 2023 13:51
@Tigrov Tigrov closed this May 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@darkdef darkdef darkdef left review comments

@terabytesoftw terabytesoftw Awaiting requested review from terabytesoftw

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Tigrov @darkdef